Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(dialog): add config option for aria-describedby #5365

Merged
merged 1 commit into from
Jul 7, 2017

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Jun 26, 2017

Adds an option to the dialog config that allows for the aria-describedby to be set on the dialog container.

@crisbeto crisbeto added Accessibility This issue is related to accessibility (a11y) pr: needs review labels Jun 26, 2017
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 26, 2017
@jelbourn
Copy link
Member

I'm not sure this is right- what motivated this change?

@crisbeto
Copy link
Member Author

crisbeto commented Jun 26, 2017

I ran into it while testing the dialog with NVDA where it doesn't read out the content at all. I based it on the example from MDN.

@jelbourn
Copy link
Member

In MDN's example, the element they're pointing to is literally a description of the dialog, though. In reality, that element will contain all sorts of arbitrary content, so it's not really accurate to say it's the describedby for the dialog element. We should support setting describedby on the dialog, but not automatically grab the entire content for it.

AFAIK the content of the dialog would be available just like any other content on the page when navigating with a screenreader. How were you moving through the dialog after opening it?

@crisbeto
Copy link
Member Author

I tabbed my way to one of the buttons and opened the dialog. The dialog header got read out correctly, as well as the button, but none of the content. I also tried tabbing and clicking around, but I couldn't get it to read out anything. FWIW, VoiceOver works as it is in master.

@jelbourn
Copy link
Member

Typically you would use NVDA's navigation to go through text content (vs. jumping between form fields with tab).

http://webaim.org/resources/shortcuts/nvda

@crisbeto
Copy link
Member Author

crisbeto commented Jun 26, 2017

You're right, I retried it and it got read out this time. I'll re-purpose the PR to add a config option for aria-describedby.

Adds an option to the dialog config that allows for the `aria-describedby` to be set on the dialog container.
@crisbeto crisbeto changed the title fix(dialog): set aria-describedby based on content element fix(dialog): add config option for aria-describedby Jun 26, 2017
@crisbeto
Copy link
Member Author

Done.

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Jun 27, 2017
@mmalerba mmalerba merged commit 68241a8 into angular:master Jul 7, 2017
amcdnl pushed a commit to amcdnl/material2 that referenced this pull request Jul 8, 2017
Adds an option to the dialog config that allows for the `aria-describedby` to be set on the dialog container.
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Accessibility This issue is related to accessibility (a11y) action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants